Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace deprecated compression options in cql #4689

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

To-om
Copy link
Contributor

@To-om To-om commented Oct 8, 2024

Closes discussion #4688


For all changes:

  • Is there an issue discussion associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue discussion number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Signed-off-by: toom <thomas@strangebee.com>
@To-om To-om force-pushed the cassandra5-compress-options branch from 3694c4a to 4b7cc43 Compare October 8, 2024 06:48
Signed-off-by: toom <thomas@strangebee.com>
@To-om
Copy link
Contributor Author

To-om commented Oct 9, 2024

ScyllaDB still uses the old options from Cassandra 2. Therefore, two different table initialization methods are needed to account for this difference.

The incompatibility of compression options has been reported here:

@@ -331,7 +331,7 @@ private static CreateTableWithOptions compressionOptions(final CreateTableWithOp
int chunkLengthInKb = configuration.get(CF_COMPRESSION_BLOCK_SIZE);

return createTable.withOption("compression",
ImmutableMap.of("sstable_compression", compressionType, "chunk_length_kb", chunkLengthInKb));
ImmutableMap.of("class", compressionType, "chunk_length_in_kb", chunkLengthInKb));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the solutions for compatibility I think:

  1. We can fetch Cassandra version before creating any tables (here): select release_version from system.local. I didn't check if ScyllaDB returns different versioning then Cassandra. If it returns only numbers then it's not useful too much, but if it returns something that defines it uniquely then we can determine if we are working with Cassandra 3, Cassandra 4, Cassandra 5, or ScyllaDB. In this case, if it's Cassandra and the version is >= 5 then we use new definition. Otherwise, we use previous definition.
  2. We can provide an optional parameter which forces usage of one or another naming regardless of what version we are working with. If so, users can specify if they want to use sstable_compression or class. As such, they can work with Cassandra 5 or any other storage which has new names for options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest version of ScyllaDB (6.2) returns the 3.0.8 when requesting the release_version. Selecting the right compression options based on this information seems to be a good option.

This reverts commit 885456f.

Signed-off-by: toom <thomas@strangebee.com>
Signed-off-by: toom <thomas@strangebee.com>
Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @To-om ! Looks good to me. I have one commend below. Also, could you please squash your commits into a single commit?

@@ -437,7 +437,7 @@ CQL storage backend options
| storage.cql.compaction-strategy-options | Compaction strategy options. This list is interpreted as a map. It must have an even number of elements in [key,val,key,val,...] form. | String[] | (no default value) | FIXED |
| storage.cql.compression | Whether the storage backend should use compression when storing the data | Boolean | true | FIXED |
| storage.cql.compression-block-size | The size of the compression blocks in kilobytes | Integer | 64 | FIXED |
| storage.cql.compression-type | The sstable_compression value JanusGraph uses when creating column families. This accepts any value allowed by Cassandra's sstable_compression option. Leave this unset to disable sstable_compression on JanusGraph-created CFs. | String | LZ4Compressor | MASKABLE |
| storage.cql.compression-type | The compression class value JanusGraph uses when creating column families. This accepts any value allowed by Cassandra's compression class option. Leave this unset to disable compression class on JanusGraph-created CFs. | String | LZ4Compressor | MASKABLE |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build-doc CI fails because in the last sentence here you specified compression class but in configuration option only compression is specified. You can execute mvn --quiet clean install -DskipTests=true -pl janusgraph-doc -am to automatically re-generate this file, so that compression class automatically turns into compression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants